Skip to content

QuerySets: check for .is_superuser instead of has_perm #8181

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 1 commit into from
May 13, 2021

Conversation

stsewd
Copy link
Member

@stsewd stsewd commented May 13, 2021

We use the is_superuser check in .com,
while we check for has_perm in .org.

These checks are only for the api user,
and has_perm always returns true for superusers.
I have checked and our api users are superusers,
so all good.

Also, user is always required.

@stsewd stsewd requested a review from a team May 13, 2021 22:32
@@ -17,7 +16,7 @@ class ProjectQuerySetBase(models.QuerySet):
use_for_related_fields = True

def _add_user_repos(self, queryset, user):
if user.has_perm('projects.view_project'):
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does anyone have these permissions on .com or .org? I assume there was a reason we did it this way, but I don't remember :x

Copy link
Member

@ericholscher ericholscher May 13, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks like we had a comment:

# Hack around get_objects_for_user not supporting global perms

https://github.com/readthedocs/readthedocs.org/pull/2954/files#diff-05611882195df182851df952b312f031a4314bce04a104e10a09655e570515f3R82

Looks like get_objects_for_user isn't used anymore, so probably safe.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Only the build user has that permission set

In [10]: User.objects.filter(user_permissions__isnull=False).distinct()
Out[10]: <QuerySet [<User: build>]>

That's in .org, in .com we have 0 users.

We use the is_superuser check in .com,
while we check for has_perm in .org.

These checks are only for the api user,
and has_perm always returns true for superusers.
I have checked and our api users are superusers,
so all good.
@stsewd stsewd force-pushed the querysets-check-superuser branch from e7f229f to 65b6d20 Compare May 13, 2021 22:40
@stsewd stsewd requested a review from a team May 13, 2021 22:55
Copy link
Member

@ericholscher ericholscher left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Makes sense to me 👍

@stsewd stsewd merged commit 4d83138 into master May 13, 2021
@stsewd stsewd deleted the querysets-check-superuser branch May 13, 2021 23:28
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants